-
Notifications
You must be signed in to change notification settings - Fork 4.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Site and Post Editor: Unify the DocumentBar component #56778
Conversation
@@ -17,8 +17,6 @@ const STORE_NAME = 'core/commands'; | |||
/** | |||
* Store definition for the commands namespace. | |||
* | |||
* See how the Commands Store is being used in components like [site-hub](https://github.com/WordPress/gutenberg/blob/HEAD/packages/edit-site/src/components/site-hub/index.js#L23) and [document-actions](https://github.com/WordPress/gutenberg/blob/HEAD/packages/edit-post/src/components/header/document-actions/index.js#L14). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed this because it gets out of sync very quickly as the code evolves.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could replace the reference to specific instances with a search query like https://github.com/search?q=repo%3AWordPress%2Fgutenberg+commandsStore+language%3AJavaScript&type=code&l=JavaScript which should always return relevant results.
Size Change: +115 B (0%) Total Size: 1.72 MB
ℹ️ View Unchanged
|
Flaky tests detected in d1eb313. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/7114315608
|
Looks good at a glance. One nuance, the document bar was recently made to be 32px tall, like so: In this branch it's 36 again: You can use $button-size-compact. |
The variable was correct but the buttons didn't have the compact prop. |
6baa7ae
to
593b841
Compare
I've noticed three things.
trunk: This PR: Third, when selecting a template part, the title seems to be incorrect. 907eff58820058c6c394051021dcb588.mp4 |
@t-hamano for the color, it was a deliberate choice from me because I felt it was a bit out of place there and random. For the other issues, they should be fixed now. |
postId | ||
); | ||
const { templateIcon, templateTitle } = useSelect( ( select ) => { | ||
const { __experimentalGetTemplateInfo: getTemplateInfo } = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I hate this selector, it's so bad in so many ways, but it's what we have today. Let's keep changes to that for later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just curious, how do you think it could be improved?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Well first it says
getTemplateInfo
and it works both for templates and template parts - I don't see why we need something like that specific to these post types but can't include all post types (to avoid the isTemplate conditionals in this component for instance)
- It takes an object, why? It should take postType, postId. A selector taking an object like that is hard to memoize... The object is probably changing way too often.
- It is also misplaced, I'm not sure why this is the "editor" package, it should be something for "core-data" maybe.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Generally looking pretty good, and a nice improvement for maintainability! Just a few minor comments below. Also, the e2e failures look to be related to the lack of space between the hidden and visible strings.
@@ -17,8 +17,6 @@ const STORE_NAME = 'core/commands'; | |||
/** | |||
* Store definition for the commands namespace. | |||
* | |||
* See how the Commands Store is being used in components like [site-hub](https://github.com/WordPress/gutenberg/blob/HEAD/packages/edit-site/src/components/site-hub/index.js#L23) and [document-actions](https://github.com/WordPress/gutenberg/blob/HEAD/packages/edit-post/src/components/header/document-actions/index.js#L14). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could replace the reference to specific instances with a search query like https://github.com/search?q=repo%3AWordPress%2Fgutenberg+commandsStore+language%3AJavaScript&type=code&l=JavaScript which should always return relevant results.
postId | ||
); | ||
const { templateIcon, templateTitle } = useSelect( ( select ) => { | ||
const { __experimentalGetTemplateInfo: getTemplateInfo } = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just curious, how do you think it could be improved?
@@ -108,17 +93,17 @@ | |||
position: absolute; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that's a design question that I'll leave out of this PR, the problem is that we also want to change the whole color on hover.
This could be addressed as a follow-up, but I noticed an issue with the document bar hiding the main inserter in the mobile viewport. This issue also occurs with trunk, but not with WP6.4. trunk: trunk.mp4WP6.4: wp6.4.mp4 |
Co-authored-by: Aki Hamano <[email protected]>
}; | ||
}, [] ); | ||
const archiveLabels = useArchiveLabel( templateSlug ); | ||
const defaultRenderingMode = useMemo( () => { | ||
return postWithTemplate ? 'template-locked' : 'all'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why memo? It's just a string?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bad habit. Thanks for noticing
Actually no, but it might be the result of the PR that introduce that regression as well. |
@youknowriad Distinguishing global vs local editing is a big pain point. We do not something that indicates the scope that you are editing. Until we find a better way (being discussed here), are we able to leave the colour for now? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense
Sure I can restore it but it does feel very out of place, I'm curious to know how many people make that connection if I'm not even capable of doing it. |
isEditingTemplate | ||
? () => | ||
setRenderingMode( | ||
getEditorSettings().defaultRenderingMode | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this limited only to template types? Navigation Menu posts (wp_navigation
) are also edited in this context and it seems like "back" default rendering mode would be useful here also. I'm just checking whether this was intentionally locked down to templates only.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh I see, this is about rendering modes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How would you feel about the default "back" action always be to go "back" in the @wordpress/router
history?
eg. history.back()
Design would like this "Back" button to become the default way to go back out of the template editing mode and I need to implement this in #55657.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The post editor doesn't use the router, so this would break.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I also feel like "back" using "history.back" is a bit redundant with the browser's back button. This "back" is more a "exist template mode" button.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for going over this again Joen. So hopefully we can agree that UI-wise we'd like the "Back" in the top toolbar to become the default UI control for that action?
@youknowriad Technically, could we add a referrer
prop to the URL which we can then consume to determine the best location for "back". If none is provided we optimise for "go back to the main site editor view"?
Obviously there would need to be an exception for situations like what is currently handled in this PR where "Back" means "Exit template editing mode" but that's simple enough. What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To be honest, it's not satisfying for me. I think we're breaking a big conceptual element in order to satisfy a small affordance.
Think of the whole editor or frame as <Editor postType={ currentPostType } postId={ currentPostId } />
. So the whole editor is just a "reusable component" that you can use in any context (post editor, site editor, elsewhere...).
Also the editor understands "template modes" so you can do <Editor postType={postType} postId={postId} templateId={templateId} />
and all the switching of modes (from post+template to template only and back...) is all internal. So in that sense, a "back" button within the editor to switch between these modes make sense.
Now, think of focus modes: focus modes mean switching the actual "postType" and "postId" passed to the editor, so they are things that are external to the editor. It's a shame to have to update the editor internally for this affordance. Also and more imporantly, it might not make sense in all these "editors" and "frames". For me, "focus mode" means navigating to another entity in the site editor, you actually switch a lot of things, not just the UI of the editor, you switch the active page in the sidebar of the site editor, you switch the URL..., you're not within the same editor anymore, it's another one. So it feels weird to have a back button within the editor. For me it's a cross page affordance and in that sense is better placed outside the editor.
Now, that is my opinion and my strong opinion here is that we shouldn't break the Editor abstraction just for this. That said, If you all feel very strongly about this "back" button, we can try to find a compromise and have a "setting" in the settings passed to EditorProvider
that contains the "back referrer" or something like that. Maybe even make it a private setting (private API)
But I persist, that I think it's wrong personally :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we agree that when users enter focus mode for a template part they need to be given an easy way back to the template they were editing?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
better placed outside the editor
Do you mean literally? The idea of a notice bar at the bottom of the viewport has come up a few times but never really gained much traction.
Worth noting theres a 'Back to x' suggestion in the command palette when you click the document bar, so I'd agree with Joen – the current placement feels natural in terms of UX.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After discussing more with @jasmussen It seems the main point of friction is that:
- we have "focus mode" for template part, navigation block and things like that where in fact we actually navigate to another page of the site editor.
- we have "template mode" in the site editor where we show a template, but we don't call that "focus mode" and we don't navigate to another page of the site editor.
If we are to treat the "back" button as similar for both these cases, they should first work similarly. In other words, maybe the "template mode" should be removed and instead we should navigate to the "template page" of the site editor instead.
Also, if we do that, and since the "template mode" is also an affordance of the "post editor", we should make sure to also add some kind of "template page" to the post editor too (maybe no url navigation there or anything but at least it should be something in the "state" of the post editor and the post editor should be able to switch).
Related #52632
What?
This PR updates the post and site editors, to use the exact same
DocumentBar
component and replaces bothDocumentActions
component that are visible in the site and post editors.To be able to do so, this PR had to switch the "template mode" of the post editor to only show the "template" without the edited post and page which matches the behavior of the site editor. (as discussed in the issue)
There are still some differences between how the modes work that I didn't update in this PR.
I left this consideration separate.
I also introduced a new
defaultRenderingMode
setting to theEditorProvider
to define the default mode that the editor needs to be in when initially rendering the current post/page/template...Testing Instructions
1- Check the template mode in the post editor, notice that it only shows the template now.
2- Check the document bar is shown properly while editing different entities in post and site editors.